Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support the affinity feature of k8s which define the rule of assigning pods to nodes #475

Merged
merged 30 commits into from
May 22, 2019

Conversation

xiaojingchen
Copy link
Contributor

@xiaojingchen xiaojingchen commented May 9, 2019

What problem does this PR solve?

  • Add affinity settings and change the nodeSelector meaning
  • Add the Configuration subject for doc operation-guide.md

What is changed and how it works?

Changed the TidbCluster struct , add the following field

Affinity         *corev1.Affinity    `json:"affinity,omitempty"`

to PDSpec/TiKVSpec/TiDBSpec and remove NodeSelectorRequired.
tidb-operator directly assigns the above Affinity field to pd/tidb/tikv's StatefulSet,and then done.

Check List

Tests

  • Manual test [done]
  • E2E test and stability test will be added in the next PR

Code changes
The following files were changed:

  • charts/tidb-cluster
  • pkg/apis/pingcap.com/v1alpha1/types.go
  • pkg/util/util.go
  • pkg/manager/member
  • docs/operation-guide.md

Side effects
Because this PR changed the TidbcCluster CRD, so it can not compatible with previous versions.

Does this PR introduce a user-facing change?:

ACTION REQUIRED: `nodeSelectorRequired` was remove from values.yaml. 
ACTION REQUIRED:  Comma-separated values support in `nodeSelector` has been dropped, please use new-added `affinity` field which has a more expressive syntax.
Support the affinity feature of k8s which can define the rule of assigning pods to nodes

@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

@xiaojingchen xiaojingchen marked this pull request as ready for review May 14, 2019 12:11
@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

@xiaojingchen
Copy link
Contributor Author

@tennix @weekface @cofyc @aylei PTAL

@xiaojingchen xiaojingchen changed the title support affinity support the affinity feature of k8s which define the rule of assigning pods to nodes May 15, 2019
@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

# podAffinityTerm:
# labelSelector:
# matchLabels:
# app.kubernetes.io/instance: <cluster.name>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# app.kubernetes.io/instance: <cluster.name>
# app.kubernetes.io/instance: <helm release name>

?

charts/tidb-cluster/values.yaml Outdated Show resolved Hide resolved
# region: cn-bj1
# Tolerations are applied to pods, and allow pods to schedule onto nodes with matching taints.
# refer to https://kubernetes.io/docs/concepts/configuration/taint-and-toleration

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add a key:

locationLabels:
- region
- zone
- rack
- host

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right,that will be added in other pr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open an issue to track it?

Co-Authored-By: weekface <weekface@gmail.com>
@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

docs/operation-guide.md Outdated Show resolved Hide resolved

* CPU & Memory

The default deployment doesn't set CPU and memory requests or limits for any of the pods, these settings can make TiDB cluster run on a small Kubernetes cluster like DinD or the default GKE cluster for testing. But for production deployment, you would likely to adjust the cpu, memory and storage resources according to the [recommendations](https://github.com/pingcap/docs/blob/master/op-guide/recommendation.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/operation-guide.md Outdated Show resolved Hide resolved
docs/operation-guide.md Outdated Show resolved Hide resolved
docs/operation-guide.md Outdated Show resolved Hide resolved
docs/operation-guide.md Outdated Show resolved Hide resolved
docs/operation-guide.md Outdated Show resolved Hide resolved
docs/operation-guide.md Outdated Show resolved Hide resolved
xiaojingchen and others added 6 commits May 16, 2019 16:31
Co-Authored-By: weekface <weekface@gmail.com>
Co-Authored-By: weekface <weekface@gmail.com>
Co-Authored-By: weekface <weekface@gmail.com>
Co-Authored-By: weekface <weekface@gmail.com>
Co-Authored-By: weekface <weekface@gmail.com>
Co-Authored-By: weekface <weekface@gmail.com>
Co-Authored-By: Tennix <tennix@users.noreply.github.com>
@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

@weekface
Copy link
Contributor

This is a breaking change. You should add these to ACTION REQUIRED of the release note.

  • nodeSelectorRequired was removed
  • nodeSelector usage changed

@weekface
Copy link
Contributor

/run-e2e-test

1 similar comment
@cofyc
Copy link
Contributor

cofyc commented May 21, 2019

/run-e2e-test

@cofyc
Copy link
Contributor

cofyc commented May 21, 2019

+ kubectl delete -f tests/manifests/e2e/e2e.yaml

service "webhook-service" deleted

clusterrolebinding.rbac.authorization.k8s.io "tidb-operator-e2e" deleted

serviceaccount "tidb-operator-e2e" deleted

Error from server: error when stopping "tests/manifests/e2e/e2e.yaml": admission webhook "check-pod-before-delete.k8s.io" denied the request without explanation

ValidatingWebhookConfigurations not cleaned in previous e2e tests?

@cofyc
Copy link
Contributor

cofyc commented May 21, 2019

ACTION REQUIRED: nodeSelectorhas a different meaning than before,the newnodeSelector just a simple key-value map and not support multiple values separated by commas.

->

ACTION REQUIRED:  Comma-separated values support in `nodeSelector` has been dropped, please use new-added `affinity` field which has a more expressive syntax.

cofyc
cofyc previously approved these changes May 21, 2019
@xiaojingchen
Copy link
Contributor Author

@cofyc ValidatingWebhookConfigurations should be cleaned in before e2e tests, the reason can not certain, I need to check e2e's clean code.
It has nothing to do with this PR.

@weekface
Copy link
Contributor

/run-e2e-tests

weekface
weekface previously approved these changes May 21, 2019
aylei
aylei previously approved these changes May 21, 2019
Copy link
Contributor

@aylei aylei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* CPU & Memory

The default deployment doesn't set CPU and memory requests or limits for any of the pods, these settings can make TiDB cluster run on a small Kubernetes cluster like DinD or the default GKE cluster for testing. But for production deployment, you would likely to adjust the cpu, memory and storage resources according to the [recommendations](https://pingcap.com/docs/dev/how-to/deploy/hardware-recommendations/#software-and-hardware-recommendations).
The resource limits should be equal or bigger than the resource requests, it is suggested to set limit and request equal to get [`Guaranteed` QoS]( https://kubernetes.io/docs/tasks/configure-pod-container/quality-service-pod/#create-a-pod-that-gets-assigned-a-qos-class-of-guaranteed).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a newline above if you want to separate these two paragraph

@xiaojingchen xiaojingchen dismissed stale reviews from aylei, weekface, and cofyc via cc14de5 May 22, 2019 08:37
@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

2 similar comments
@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

@xiaojingchen
Copy link
Contributor Author

/run-e2e-test

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants